Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove prev-minor testing as not needed anymore #30926

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Remove prev-minor testing as not needed anymore #30926

merged 1 commit into from
Mar 24, 2022
+0 −56

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Mar 21, 2022

In #26629 the issue around running Filebeat against older version of Elasticsearch was discussed and in #28274 testing against the previous minor was introduced. But since 8.0, Filebeat can only ship data to equal or newer versions of Elasticsearch. Because of this, in the tests TESTING_FILEBEAT_ALLOW_OLDER=1 had to be introduced.

I'm opening this PR to remove this testing as it is not something we support anymore.

@ruflin
Remove prev-minor testing as not needed anymore
129c75d 
In elastic#26629 the issue around running Filebeat against older version of Elasticsearch was discussed and in elastic#28274 testing against the previous minor was introduced. But since 8.0, Filebeat can only ship data to equal or newer versions of Elasticsearch. Because of this, in the tests `TESTING_FILEBEAT_ALLOW_OLDER=1` had to be introduced.

I'm opening this PR to remove this testing as it is not something we support anymore.
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 21, 2022
@ruflin ruflin self-assigned this Mar 21, 2022
@ruflin ruflin requested review from adriansr and andrewkroh March 21, 2022 07:46
@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2022

This pull request does not have a backport label. Could you fix it @ruflin? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Mar 21, 2022
@ruflin ruflin added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Mar 21, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 21, 2022
@andresrc
Copy link
Contributor

I understand this would not be backported to 7.x.

@ruflin
Copy link
Contributor Author

ruflin commented Mar 21, 2022

No plans to backport at all.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-03-21T07:46:23.390+0000

  • Duration: 136 min 7 sec

Test stats 🧪

Test Results
Failed 0
Passed 23780
Skipped 2105
Total 25885

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

andrewkroh
andrewkroh approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this check seems aligned to the language in the support matrix (though I would have preferred stronger language for a contract such that it was required rather than "highly recommended").

Screen Shot 2022-03-23 at 12 23 02

And then users also would need to set allow_older_versions: true (https://www.elastic.co/guide/en/beats/filebeat/8.1/elasticsearch-output.html#_allow_older_versions) to be able to get into a situation where these's test would have caught the problem.

There's a bunch of code that that makes pipeline configs backwards compatible with earlier ES versions. That's now eligible for removal.

@ruflin
Copy link
Contributor Author

ruflin commented Mar 24, 2022

@nimarezainia I'm also surprised by the phrasing. Can you take a look at this?

@andrewkroh I'll get the PR merged. It is great to hear that it will actually simplify some of our code. As all the checks are for 7.x it seems we can get rid of all? Will you follow up or should the data plane team take care of it?

@ruflin ruflin merged commit ba5336a into elastic:main Mar 24, 2022
@ruflin ruflin deleted the remove-prev-minor branch March 24, 2022 09:22
@nimarezainia
Copy link
Contributor

@nimarezainia I'm also surprised by the phrasing. Can you take a look at this?

@andrewkroh I'll get the PR merged. It is great to hear that it will actually simplify some of our code. As all the checks are for 7.x it seems we can get rid of all? Will you follow up or should the data plane team take care of it?

sure I can change that. I worded it that way mainly because there is a "public" config that allows the user to over write. So clearly we are not that strict :-)

@ruflin
Copy link
Contributor Author

ruflin commented Mar 29, 2022

The config we have is experimental and we discourage it from using it. "Highly recommended" sounds to me like we would support the config option, but we don't.

kush-elastic pushed a commit to kush-elastic/beats that referenced this pull request May 2, 2022
@ruflin @kush-elastic
Remove prev-minor testing as not needed anymore (elastic#30926)
604e40d 
In elastic#26629 the issue around running Filebeat against older version of Elasticsearch was discussed and in elastic#28274 testing against the previous minor was introduced. But since 8.0, Filebeat can only ship data to equal or newer versions of Elasticsearch. Because of this, in the tests `TESTING_FILEBEAT_ALLOW_OLDER=1` had to be introduced.

I'm opening this PR to remove this testing as it is not something we support anymore.
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
@ruflin @chrisberkhout
Remove prev-minor testing as not needed anymore (#30926)
5aec3ba 
In #26629 the issue around running Filebeat against older version of Elasticsearch was discussed and in #28274 testing against the previous minor was introduced. But since 8.0, Filebeat can only ship data to equal or newer versions of Elasticsearch. Because of this, in the tests `TESTING_FILEBEAT_ALLOW_OLDER=1` had to be introduced.

I'm opening this PR to remove this testing as it is not something we support anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

@adriansr adriansr Awaiting requested review from adriansr

Assignees

@ruflin ruflin

Labels
backport-skip Skip notification from the automated backport with mergify Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ruflin @elasticmachine @andresrc @nimarezainia @andrewkroh